Reconcile ReleaseHolder, ComHolderPreemp, and ComHolderAnyMode - #130373
Reconcile ReleaseHolder, ComHolderPreemp, and ComHolderAnyMode#130373AaronRobinsonMSFT wants to merge 6 commits into
Conversation
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR refactors CoreCLR’s COM / ref-counted RAII “holder” usage to converge on the LifetimeHolder<Traits> pattern: replacing legacy SpecializedWrapper APIs (Extract, Clear, GetValue, etc.), removing ComHolderPreemp, and renaming ComHolderAnyMode to ReleaseHolderAnyMode.
Changes:
- Retargets
ReleaseHolderto aLifetimeHolder<ReleaseHolderTraits<T>>implementation and introducesHolderDetail::HasReleaseMethodfor duck-typedRelease()detection. - Renames / consolidates COM holders (
ComHolderAnyMode→ReleaseHolderAnyMode; removesComHolderPreemp) and migrates call sites toDetach()/Free()patterns. - Refactors a few lifetime/ownership patterns (notably
AppDomain::BindAssemblySpec) to avoid relying on legacy holder APIs.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/wrappers.h | Renames ComHolderAnyModeTraits/alias to ReleaseHolderAnyMode* and removes ComHolderPreemp traits. |
| src/coreclr/vm/weakreferencenative.cpp | Migrates ComHolderPreemp<> usages to ReleaseHolder<>. |
| src/coreclr/vm/stubmgr.cpp | Migrates ComHolderAnyMode<> usages to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/stubhelpers.cpp | Migrates ComHolderAnyMode<> usages to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/stdinterfaces.cpp | Migrates COM holders to ReleaseHolder<> / ReleaseHolderAnyMode<>. |
| src/coreclr/vm/runtimecallablewrapper.cpp | Migrates COM holders and adjusts a CCWHolder lifetime flow to use Detach(). |
| src/coreclr/vm/rejit.cpp | Removes legacy = NULL initialization pattern for ReleaseHolder. |
| src/coreclr/vm/peimagelayout.cpp | Replaces Extract() with Detach() when returning owned layouts. |
| src/coreclr/vm/peimage.cpp | Migrates metadata COM holder to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/peassembly.cpp | Migrates ComHolderPreemp<> to ReleaseHolder<>. |
| src/coreclr/vm/olevariant.cpp | Migrates COM holders to ReleaseHolder<> / ReleaseHolderAnyMode<>. |
| src/coreclr/vm/olecontexthelpers.cpp | Migrates ComHolderPreemp<> to ReleaseHolder<>. |
| src/coreclr/vm/nativeimage.cpp | Replaces legacy holder APIs (IsNull/Extract) with == NULL and Detach(). |
| src/coreclr/vm/marshalnative.cpp | Updates CCWHolder initialization to brace-init and adjusts holder usage. |
| src/coreclr/vm/interoputil.cpp | Call sites migrated to new holder type names. |
| src/coreclr/vm/interopconverter.cpp | Migrates COM holders and replaces legacy holder APIs in a few places. |
| src/coreclr/vm/encee.cpp | Renames holder types in EnC path comments and variables. |
| src/coreclr/vm/eetoprofinterfaceimpl.cpp | Replaces Extract()+NULL patterns with Detach() for ownership transfer. |
| src/coreclr/vm/dllimport.cpp | Updates holder assignment to use operator= instead of Assign. |
| src/coreclr/vm/dispparammarshaler.cpp | Migrates COM holders to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/crossloaderallocatorhash.inl | Updates holder initialization style and uses Detach() for returns. |
| src/coreclr/vm/coreassemblyspec.cpp | Uses Detach() when returning owned binder assembly. |
| src/coreclr/vm/commodule.cpp | Migrates COM holders to ReleaseHolder<> / ReleaseHolderAnyMode<>. |
| src/coreclr/vm/comconnectionpoints.cpp | Migrates ComHolderAnyMode<> to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/comcallablewrapper.cpp | Migrates holders and refactors template-return ownership to use Detach(). |
| src/coreclr/vm/comcache.cpp | Migrates COM holders and replaces Extract() with Detach(). |
| src/coreclr/vm/clrex.cpp | Migrates error-info holder to ReleaseHolderAnyMode<>. |
| src/coreclr/vm/ceeload.cpp | Migrates symbol reader holders to ReleaseHolder<> and modernizes initialization. |
| src/coreclr/vm/assemblynative.cpp | Passes holder directly to tracing (via implicit pointer conversion). |
| src/coreclr/vm/assembly.cpp | Uses Detach() for ownership transfer and brace-init for holders. |
| src/coreclr/vm/appdomain.hpp | Adjusts FindCachedFile signature (removes unused fThrow). |
| src/coreclr/vm/appdomain.cpp | Refactors BindAssemblySpec lifetime management; simplifies FindCachedFile. |
| src/coreclr/md/enc/mdinternalrw.cpp | Fixes QueryInterface call to pass void**. |
| src/coreclr/inc/holder.h | Removes legacy ReleaseHolder wrapper, adds LifetimeHolder-based ReleaseHolder + HasReleaseMethod. |
| src/coreclr/dlls/mscoree/exports.cpp | Uses Detach() to transfer host handle ownership. |
| src/coreclr/debug/ee/debugger.cpp | Migrates ComHolderPreemp<> to ReleaseHolder<>. |
| src/coreclr/debug/daccess/enummem.cpp | Replaces legacy holder APIs (Clear/GetValue) with Free() and direct holder conversions. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Removes = nullptr initialization for ReleaseHolder. |
| src/coreclr/debug/daccess/daccess.cpp | Removes = nullptr initialization for ReleaseHolder. |
| src/coreclr/debug/daccess/cdac.h | Replaces Extract() with Detach() in move operations. |
| src/coreclr/debug/createdump/threadinfo.cpp | Replaces Extract() with Detach() when transferring method ownership into a frame. |
| src/coreclr/debug/createdump/createdumpunix.cpp | Updates holder initialization to brace-init. |
| src/coreclr/debug/createdump/crashinfo.cpp | Removes = nullptr initialization for ReleaseHolder. |
| src/coreclr/binder/defaultassemblybinder.cpp | Replaces Extract() with Detach() for binder assembly ownership transfer. |
| src/coreclr/binder/customassemblybinder.cpp | Replaces Extract() with Detach() for binder assembly ownership transfer. |
| src/coreclr/binder/assemblybindercommon.cpp | Replaces Extract() with Detach() for binder assembly ownership transfer. |
| src/coreclr/binder/assembly.cpp | Replaces Extract() with Detach() for assembly-name ownership transfer. |
Copilot's findings
- Files reviewed: 47/47 changed files
- Comments generated: 4
676775a to
f4722e7
Compare
- Replaced instances of ComHolderAnyMode with ReleaseHolder and ReleaseHolderAnyMode across multiple files to enhance memory management and ensure proper release of COM interfaces. - Updated the ListLockEntryHolder to use LifetimeHolder with custom traits for better encapsulation of resource management. - Removed unnecessary comments and clarified code where applicable, particularly in EEContract files. - Improved consistency in handling COM interface pointers, ensuring that ownership semantics are clear and that resources are released appropriately. - Adjusted various methods in interop and runtime files to utilize the new holder classes, ensuring that the code adheres to modern C++ practices for resource management.
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/vm/wrappers.h:59
ReleaseHolderAnyModeTraitscallsSafeRelease, which is specifically forIUnknown*and performs a GC-mode transition. The updated comment says this holder works for any type with aRelease()method, and the priorIUnknownconstraint (static_assert(std::is_base_of<IUnknown, TYPE>::value)) was removed. This makes it easier to accidentally useReleaseHolderAnyModefor non-COM types and get either confusing compile errors or incorrect assumptions about GC-mode behavior.
Consider restoring the IUnknown constraint and updating the comment back to explicitly describe a COM interface holder.
- Files reviewed: 62/63 changed files
- Comments generated: 1
f4722e7 to
9d9b795
Compare
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/vm/wrappers.h:60
- ReleaseHolderAnyModeTraits dropped the compile-time constraint that TYPE must be a COM interface (IUnknown-derived), but Free() still calls SafeRelease(IUnknown*), so non-IUnknown types will fail to compile with a less obvious error. Keeping the static_assert makes misuse failures clearer and aligns with SafeRelease’s actual signature.
- Files reviewed: 62/63 changed files
- Comments generated: 1
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/coreclr/inc/holder.h:1003
- ReleaseHolderTraits::Free() only uses STATIC_CONTRACT_* macros, which are no-ops even when contracts are enabled. Since ReleaseHolder is now used throughout the VM to represent the preemptive-mode release holder, it would be better to preserve runtime contract checking (at least MODE_PREEMPTIVE) when ENABLE_CONTRACTS_IMPL is defined, while keeping the static annotations for non-VM consumers.
static void Free(Type value)
{
STATIC_CONTRACT_NOTHROW;
STATIC_CONTRACT_GC_TRIGGERS;
STATIC_CONTRACT_MODE_PREEMPTIVE;
src/coreclr/vm/wrappers.h:59
- ReleaseHolderAnyModeTraits::Free() calls SafeRelease(IUnknown*), so the trait still effectively requires TYPE to derive from IUnknown. Re-adding the static_assert improves diagnostics and keeps the template constraint explicit (the current comment also suggests non-COM types would work, but they won’t with SafeRelease).
src/coreclr/vm/wrappers.h:75 - The comment for ReleaseHolderAnyMode says it works for any type with a Release() method, but the implementation calls SafeRelease (which is typed to IUnknown*). Updating the comment will avoid implying support for non-COM types.
- Files reviewed: 62/63 changed files
- Comments generated: 0 new
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/inc/holder.h:1007
- ReleaseHolderTraits::Free() currently uses only STATIC_CONTRACT_* annotations, which are no-ops (see staticcontract.h). That means contract-enabled VM builds won’t enforce the intended MODE_PREEMPTIVE requirement (unlike the removed ComHolderPreempTraits which used CONTRACTL { MODE_PREEMPTIVE; }). If the intent is to catch accidental cooperative-mode COM Release() calls in the VM, this should use a real CONTRACTL under ENABLE_CONTRACTS_IMPL and fall back to STATIC_CONTRACT_* otherwise.
template <typename TYPE>
struct ReleaseHolderTraits final
{
using Type = TYPE*;
static constexpr Type Default() { return NULL; }
static void Free(Type value)
{
STATIC_CONTRACT_NOTHROW;
STATIC_CONTRACT_GC_TRIGGERS;
STATIC_CONTRACT_MODE_PREEMPTIVE;
if (value != NULL)
value->Release();
}
src/coreclr/vm/wrappers.h:75
- The comment says this holder works for any held type with a Release() method, but the implementation calls SafeRelease(value), which requires the held pointer to be convertible to IUnknown*. This is effectively a COM interface holder; the comment should reflect that to avoid accidental misuse.
- Files reviewed: 62/63 changed files
- Comments generated: 0 new
…Traits; add GCX_PREEMP in GetITypeInfoForEEClass
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/coreclr/inc/holder.h:1002
- ReleaseHolderTraits::Free declares STATIC_CONTRACT_GC_NOTRIGGER, but Release() on COM interfaces (and other ref-counted objects) can trigger GC/message pumping (e.g., SafeReleasePreemp is annotated GC_TRIGGERS). The current contract is too strict and can cause contract violations in checked builds.
STATIC_CONTRACT_GC_NOTRIGGER;
src/coreclr/vm/wrappers.h:75
- This comment says the holder releases a "type with a Release() method", but ReleaseHolderAnyModeTraits::Free calls SafeRelease(value), whose signature takes IUnknown*. This holder is COM/IUnknown-specific, so the comment should reflect that to avoid accidental misuse.
- Files reviewed: 63/64 changed files
- Comments generated: 1
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/inc/holder.h:1004
- ReleaseHolderTraits::Free is marked GC_NOTRIGGER and lacks the same contract-violation allowances as SafeReleasePreemp, but it calls IUnknown::Release directly (which can message-pump and run arbitrary managed code). This can lead to contract assertions / incorrect assumptions in callers that replaced ComHolderPreemp with ReleaseHolder.
STATIC_CONTRACT_NOTHROW;
STATIC_CONTRACT_GC_NOTRIGGER;
STATIC_CONTRACT_MODE_PREEMPTIVE;
- Files reviewed: 63/64 changed files
- Comments generated: 1
Converge the three COM-interface RAII holders (
ReleaseHolder,ComHolderPreemp,ComHolderAnyMode) onto theLifetimeHolder<Traits>pattern.Changes
ReleaseHolderis retargeted toLifetimeHolder<ReleaseHolderTraits<T>>, adoptingComHolderPreemp's preemptive-mode release semantics (the VM default).Free()uses aMODE_PREEMPTIVEcontract gated onENABLE_CONTRACTS_IMPL, so non-VM consumers (utilcode, md, DAC, createdump) fall back to a static-contract annotation.ComHolderPreempis removed;ReleaseHolderreplaces all uses.ComHolderAnyModeis renamed toReleaseHolderAnyMode.